ON-16435: Replace arch checks with nic capabilies#75
Draft
ligallag-amd wants to merge 3 commits intomasterfrom
Draft
ON-16435: Replace arch checks with nic capabilies#75ligallag-amd wants to merge 3 commits intomasterfrom
ligallag-amd wants to merge 3 commits intomasterfrom
Conversation
This makes it easier to change whether these flags are in zf_stack or zf_stack_impl. Keeping them in the latter struct may have impact on cache hit rate in the hotpath where these flags may be queried.
sianj-xilinx
reviewed
Apr 3, 2025
| pkt_id next_packet_id = 0; | ||
| char* next_packet; | ||
| bool is_efct = vi->nic_type.arch == EF_VI_ARCH_EFCT; | ||
| bool is_rx_ref = *zf_stack_res_nic_flags(st, nic) & |
Collaborator
There was a problem hiding this comment.
You're using these flags on the critical path. The zf_stack_impl is used for cold path state. If we need the checks on the hot path that information should move to zf_stack_nic. That might just be the RX_REF capability - probably needs a bit of care to ensure that this is done in a way that doesn't impact performance.
sianj-xilinx
reviewed
Apr 3, 2025
Comment on lines
290
to
+291
| zf_assert_nequal(vi->nic_type.arch, EF_VI_ARCH_EFCT); | ||
| zf_assert_nequal(vi->nic_type.arch, EF_VI_ARCH_EF10CT); |
Collaborator
There was a problem hiding this comment.
Assert that CTPIO_ONLY capability wasn't set?
Comment on lines
+19
to
+20
| if( vi->vi_flags & EF_VI_TX_CTPIO && vi->nic_type.arch != EF_VI_ARCH_EFCT | ||
| && vi->nic_type.arch != EF_VI_ARCH_EF10CT ) { |
Collaborator
There was a problem hiding this comment.
The check here is on whether we need to post a dma fallback, which we don't if we have the CTPIO_ONLY capability, so I think that's what we should use.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The final commit is probably not what we want but I am at a bit of a loss for what capabilities should be used in those cases. I therefore added ef10ct arch checks. Opening this as a draft because of this.
I'm also wondering about the performance implications of this change. It queries the flags in the hot path of RX and TX. I wonder if it's therefore worth moving the location of these flags into
zf_stack::nic. Or perhaps we should usevi::nic_type::nic_flagsinstead for the capability checks.Testing C-Model
zfsinkzfsendAll packets received at the other end. I had to modify zfsend to sleep 1000us between sends (not sure of the sends are batched) because otherwise the sends would fail and zfsend would hit an assertion. This is fine because the C-Model cannot keep up with the sends.